ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Generated by 🚫 dangerJS

Purely from a DevTools perspective, this change looks okay (since we call onCommitFiberUnmount before this clean up happens). Haven't considered beyond that scope yet.

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

This helps remove memory leaks where effect chains point to old nodes

I'm not sure I agree with describing this as a "memory leak" since there are only ever two fibers (current and alternate). Nothing is "leaked", just "retained" for longer than necessary in some cases.

(Unless maybe there's context here that I'm unaware of.)

@gaearon I'm having trouble creating an isolated repro but I can share a real world example.

Repro steps

1. Log into Discord (https://discordapp.com/app) in your browser.
2. Click this link. This puts you in a custom build with react-dom NPM 16.8.4.
3. You can confirm custom build by hovering over gear icon in lower right.
   
4. Go into a server (example). Repeatedly switch between two text channels in a server while profiling memory usage. See memory usage go up a lot.

Repro with this PR

Repeat with this custom build which uses react-dom from this PR.

See that memory is freed when you switch between channels.

Investigation

When you have a Fiber node reference, it links to all the other nodes in that tree as well as old host nodes via stateNode.

@bvaughn Regarding "memory leak", I've found instances where mounted nodes have nextEffect references to unmounted nodes. This seems possible if a subset of the React tree changes (e.g. one pane of many). Components outside the changed tree are not touched and can continue to hold nextEffect pointers to old nodes within the changed tree. In Discord, the sidebar does not update when you change URL routes from one channel to another within the same server. I confirmed this by observing nextEffect.

Does this sound plausible? I'd love to get an isolated repro and will keep trying. Thanks for looking!

I think we should probably reset the nextEffect pointer to null during the last traversal over the effect list in the commit phase (most likely passive effects).

This gets more complex in concurrent mode renders can be interrupted tho.

@sebmarkbage The PR, as is, takes the conservative approach of removing only Deletion effects from the nextEffect chain. It's done in commitAllHostEffects, the last place deletion effects are needed.

This is an optimization. If we're okay doing another traversal to unwind all nextEffect pointers, I'm happy to do that. If there are passive effects, we can unwind in that traversal. Otherwise, do another traversal.

@acdlite looks like there's an active Scheduler rewrite! It might make this PR moot but I think unwinding nextEffect pointers is still relevant in the new version.

@acdlite can you confirm that #15196 will ultimately resolve this issue? it's not obvious to me that it does in current draft form, but it's a draft after all :)

@sebmarkbage @gaearon I've updated this to clear all nextEffect pointers after they're no longer needed (either flushing passive effects or after committing layout effects). This seems simpler than removing only Deletion nodes.

I've also updated this to handle the new fiber scheduler. Let me know if there's anything I can do to move this forward. Thanks!

@sebmarkbage pointed me to this thread so I thought I'd share this publicly FWIW:

I recently upgrade the WhatsApp web client from React 16.3.1 to 16.8.4 and we got a huge increase in memory consumption: every conversation component we showed was retained forever. We ended up tracing this back to our animation library holding on to a reference to two DOM nodes. These nodes referenced a linked list of Fibers (via __reactInternalInstance$jf6dr7u9fbf), each of which held on to a component instance.

While it doesn't exactly mirror what's going on in our app, I managed to create a minimal repro:

https://codepen.io/matthewwithanm/pen/LvmqJP

I get that this isn't technically a memory leak on React's part, but React made a tiny, very bounded leak (2 nodes) into an unbounded one that scaled linearly with the number of screens visited (!)

We're seeing the same thing as @matthewwithanm points out above, applying this PR on a local fork solved the memory leak issue for us.

Also confirming that this patch resolves the memory leak that we are experiencing in our application. @sebmarkbage the information here and posted in the umbrella issue has lead to us doing a lot to mitigate the impact of the leak, so thank you for the guidance on what can compound the issue. Still, we feel we are chipping away slowly at the problem, and it’s all worth while to do even in the absence of this issue, but with a looming launch date we are anxiously awaiting this fix.

@brad-decker Mind outlining what application pattern actually caused the leak similar to how others have posted a screenshot from the Chrome heap snapshots? Maybe you're still leaking large DOM trees and maybe we can help you fix that too rather than just mitigate the leak.

@sebmarkbage absolutely thank you.

We use Nextjs and discovered this issue because on mobile when a user flips back and forth between two of our pages, ones that are a real-world use case / hot path of user traffic, the page crashes. The number of dom nodes in memory is increasing over time with each page view.

I am very novice at using the heap snapshot to determine what are the culprits of memory retention, but so far we have discovered a lot of the issues due to realizing that we were using hooks inappropriately and closing over refs and state. We fixed a lot of this via the exhaustive deps rule, but i'm sure we are still doing some things in various files that are not helping the situation.

What i've done to diagnose the issue:

1. I chose two pages in our application and removed all of the layout that encapsulates them, to verify it wasn't anything higher than the page level holding onto memory. In Next land that means I removed my _app and _document so that each page is isolated.
2. I added links at the top of the page from one page to the other.
3. Flipping back and forth between pages, taking snapshots, hunting down retainers and figuring out why they are holding onto nodes.
4. Removed whole sections of the page entirely to see if i could isolate components that were more problematic than others, etc.

I would love some guidance on how to best work through this. I see 'stateNode' and 'nextEffect' as common retainers of memory but i'm not familiar enough with the architecture to understand the significance of these items.

I have a heapshapshot i can send if it would help more than screenshots. Again pretty novice here.

@brad-decker This is actually super helpful because this is showing a new kind of leak that we haven't seen before in this thread. It looks like a closure that is closing over an internal React Fiber data structure. That is interesting because you probably aren't getting a handle to it yourself. Most likely it's React closing over its own internals. It only does this one case: The dispatch function returned from useReducer which is also used to implement setState from useState.

My guess is that you're leaking a dispatch or setState function in an event listener that you're manually attaching to the window (e.g. in an effect). This would be common in things listening to global key press events or click-outside solutions. You might be using a third party tool that does that.

An example that would cause this:

let [value, setValue] = useState(null);
useEffect(() => {
  window.addEventListener('keyup', () => {
    setValue(...);
  });
});

This is a pretty bad leak that you probably want to fix anyway since you don't want keypresses to potentially cause unexpected side-effects due to this leak.

However, it is super helpful to me because it's a pretty common bug. It's something we can try to add monitoring and logging for, and we can find a fix that is stronger than the one proposed in this PR.

@sebmarkbage the only success I have had in totally resolving our memory issues is by removing the Link component from next js everywhere. I have had a few false positives in my journey so I'm really not sure if I'm on the right track.

https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx

Does this seem like a good rabbit hole to you?

EDIT: doesn't seem to be the only or even the primary issue.

export const useDimensions = (
  ref: Object,
  throttleTime?: number = 100,
): UseDimensionsReturnType => {
  /**
   * This can be extended to return scroll position and other
   * attributes as needed
   */
  const [height, setHeight] = useState(0);
  const [width, setWidth] = useState(0);
  const [offsetHeight, setOffsetHeight] = useState(0);
  const [offsetWidth, setOffsetWidth] = useState(0);

  const handler = useCallback(() => {
    if (ref.current) {
      const { height, width } = ref.current.getBoundingClientRect();
      const { offsetHeight, offsetWidth } = ref.current;
      setHeight(height);
      setWidth(width);
      setOffsetHeight(offsetHeight);
      setOffsetWidth(offsetWidth);
    }
  }, [setHeight, setOffsetHeight, setWidth, ref, setOffsetWidth]);

  // useMemo to prevent creating this function more than once
  useEffect(() => {
    window.addEventListener('resize', handler);
    return () => {
      window.removeEventListener('resise', handler);
    };
  }, [handler]);

  return { height, width, offsetHeight, offsetWidth };
};

i've moved the calculation around to in the effect, outside the effect, etc, and can't seem to figure this out. setOffsetHeight is retaining dom nodes for sure, and this is our code. Any advice?

  // useMemo to prevent creating this function more than once
  useEffect(() => {
    window.addEventListener('resize', handler);
    return () => {
      window.removeEventListener('resise', handler);
    };
  }, [handler]);

hey @brad-decker! it looks like "resize" is spelled wrong in the removeEventListener() call so it's never actually being removed! (one reason why the EventTarget API is less than 💯)

hey @brad-decker! it looks like "resize" is spelled wrong in the removeEventListener() call so it's never actually being removed! (one reason why the EventTarget API is less than 💯)

That would cause a leak to everything in that function's scope right, since window would have a reference.

Well, dang. Thanks for pointing that out! I am kicking myself for that typo :P

#14387 reading through this and trying to understand the implications of accessing a ref in an effect on memory leaks. The ref is a mutable pointer but its part of the component scope, yeah? Do we put refs in the dependency array of an effect? Using my previous example, the one with the typo, if the handler was created in the effect as per the recommendation in the hooks FAQ for dealing with function, wouldn't I need to also include ref as a dependency? Same question, i suppose, with useCallback?

Do we put refs in the dependency array of an effect?

No, it's not necessary to specify refs in the dependency array because refs are expected to be mutated by user code (outside of React, without triggering a re-render).

@sebmarkbage @bvaughn @timneutkens @Timer it turns out that we have a compounding problem with Next and React. The code snippet above with the unfortunate typo was a result of me trying to hack to find the memory leak, that code did not exist prior. While it did help me from continuing to go down a rabbit hole that was caused by my own typo in my search for the underlying leak, that was not the root cause.

on line: https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L101 of next/link an element is set as the key of a map, this element comes from a ref passed to a child component by the Link component. Link basically forwards props to its child so we can add Next magic to 'a' tags or even 'div' tags. The problem seems to be that either next/link itself is closing over a fiber node by doing this, or something in our code that is adjacent to the Links in some way is holding onto the node.

How I have confirmed this:

1. I checked out our master branch to make sure I was in the original state of our memory leak.
2. in node_modules I found the link file and deleted the listeners.set call, and nothing else.
3. ran performance recording for 25 seconds, and during twenty of those seconds, I flipped back and forth between two pages.

before

after

we still have minor memory leaks in our application, as illustrated by the gradual upwards trend, but it's significantly less prominent now with very clear successful garbage collecting without the listener being set in the map and no code change on our end.

thoughts?

This line in Link makes me skeptical: https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L117

componentDidMount fires after the ref callbacks of children. The idea is that they need to be wired before they can be referenced in componentDidMount.

https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L124

So that would imply that the clean up function gets overridden before it can be called.

Another issue is that makes this code a bit fragile is if handleRef gets called multiple times it doesn't clean up the old ones. E.g. if props switches or something. I don't see that happening now but seems like it would be easy to make such as mistake.

Having @ijjk look into it!

Update, tested the above application against changes made in vercel/next.js#7943 by @ijjk and it seems to be resolved. We have also started fixing a myriad of "minor" leaks that have an additive effect. All of them are related to things already brought up by @sebmarkbage in this and the umbrella thread. Thanks for all of the assistance, @sebmarkbage @bvaughn @matthewwithanm @timneutkens and @ijjk

Just to note for folks following this thread (as the information can be easy to miss), the author of this PR opened another PR (#16115) to address part of the leak.

This was merged in last week and looks to help my application's memory usage a lot better.

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

My application memory leak is for this reason

Edit: This does seem to fix quite a lot.

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

When will I plan to merge this?

I am noticing a large disparity between the memory leak in my application when NODE_ENV=development versus NODE_ENV=production.

Here is the development leak:

Here is the production leak:

This is a profile of twice loading/unloading a particular tree in an internal application. I've focused on two spikes which consistently appear and continue to appear regardless of how many times I unload/reload the vertical tree. As you can see, the leak is much worse in production (2MB vs 18MB). This profile was run on 568ef3e.

EDIT: More details here.

I tried using this fork to resolve some memory leak issues I've been having but I actually ended up getting better results with 16.9 than with this. For a bit of context, I have an app with react router where switching between two components will lead to an accumulation of stale fiber nodes.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!